Port 14.4 - 14.20 changes to REL_2_STABLE, p1 #1574
Port 14.4 - 14.20 changes to REL_2_STABLE, p1 #1574reshke wants to merge 12 commits intoapache:REL_2_STABLEfrom
Conversation
|
Hi @reshke , I suggest not rushing to cherry-pick the commits to the REL_2_STABLE branch. We can do that in a batch with the next release. |
Okay. So, what will be the batch size? Is it all 14.4 - 14.20 commits, all commits that did make it to main, or 14.4 - 14.5 portion (maybe some other rule?). I will update this PR incrementally, then. And before next release, we will rebase & commit |
It sounds make sense. Thanks. |
|
Sorry, I don't see here commit eacd9c7 It is relevant REL_2_STABLE, could we merge it to REL_2_STABLE too? |
reshke
force-pushed
the
rel_2_1566p1
branch
2 times, most recently
from
34a0430 to
06cc38c
Compare
|
I have a thought about the cherry-pick strategy for Instead of picking individual commits, would it make sense to start from the commits after the previous cherry-pick PR (#1547) and move forward from there? My concern is that selecting commits piecemeal might make it harder to manage in the next release cycle, since more commits will be added in between and it may become less clear which ones were already backported. If this approach makes sense, we could also keep this PR and use it as the cherry-pick PR for the next release. |
Hi! I did not get your idea... As of today, the last agreement we met in Apache Cloudberry dev list was [0], which is: So, after this PR there will be no contribution to main branch which is 14.4-14.20 range, so nothing to back-patch. Kernel upgrade will continue in REL_2_STABLE only, tracked here[1]. If I am mistaken is something please clarify .... [0] https://lists.apache.org/thread/bkrx7470kk08kvz2t7rv68gll1sp67vm [1] https://docs.google.com/spreadsheets/d/1vjjEb39QPyXO-nDJZ0tHAtReIQKka0S2YnD2r1AFhkk |
|
Hi @reshke, let me clarify my previous comment. My concern is about the cherry-pick strategy for the upcoming release (e.g., For easier management, I suggest we start from the point where the last
My suggested approach would ensure we don't miss any commits that should be included in the next release, and it's easier to track what has been backported. Just want to make sure, does this sound good to you? |
I have created separate PR for this #1610 |
Sorry, I messed up with #1610, here is another try #1611 |
Take it easy. Your approach looks great to me! Thanks for your excellent work. It would be better to list the excluded commits that are not cherry-picked for |
The code tried to access ARR_DIMS(v)[0] and ARR_LBOUND(v)[0] whether or not those values exist. This made the range check on the "n" argument unstable --- it might or might not fail, and if it did it would report garbage for the allowed upper limit. These bogus accesses would probably annoy Valgrind, and if you were very unlucky even lead to SIGSEGV. Report and fix by Martin Kalcher. Back-patch to v14 where this function was added. Discussion: https://postgr.es/m/baaeb413-b8a8-4656-5757-ef347e5ec11f@aboutsource.net
Commit fac1b47 thought we could check for set-returning functions by testing only the top-level node in an expression tree. This is wrong in itself, and to make matters worse it encouraged others to make the same mistake, by exporting tlist.c's special-purpose IS_SRF_CALL() as a widely-visible macro. I can't find any evidence that anyone's taken the bait, but it was only a matter of time. Use expression_returns_set() instead, and stuff the IS_SRF_CALL() genie back in its bottle, this time with a warning label. I also added a couple of cross-reference comments. After a fair amount of fooling around, I've despaired of making a robust test case that exposes the bug reliably, so no test case here. (Note that the test case added by fac1b47 is itself broken, in that it doesn't notice if you remove the code change. The repro given by the bug submitter currently doesn't fail either in v15 or HEAD, though I suspect that may indicate an unrelated bug.) Per bug #17564 from Martijn van Oosterhout. Back-patch to v13, as the faulty patch was. Discussion: https://postgr.es/m/17564-c7472c2f90ef2da3@postgresql.org
Per suggestion from Robert Haas Backpatch to v14 Discussion: https://www.postgresql.org/message-id/CA%2BTgmoZ1QvHquYHLkMy1oHKqz4-E7QQctj6e0ocq_GP1B5%2B9bA%40mail.gmail.com
Erik Rijkers and Justin Pryzby Backpatch to v14 Discussion: https://www.postgresql.org/message-id/b79bfeff-d0e3-29a3-2576-0e325848dede%40xs4all.nl
reshke
force-pushed
the
rel_2_1566p1
branch
2 times, most recently
from
e42b84b to
c8a9598
Compare
Remove the test case added by commit fac1b47, which never actually worked to expose the problem it claimed to test. Replace it with a case that does expose the problem, and also covers the SRF-not- at-the-top deficiency repaired in 1aa8dad. Richard Guo, with some editorialization by me Discussion: https://postgr.es/m/17564-c7472c2f90ef2da3@postgresql.org
When inserting a view referencing a foreign table that has WITH CHECK OPTION constraints, in single-insert mode postgres_fdw retrieves the data that was actually inserted on the remote side so that the WITH CHECK OPTION constraints are enforced with the data locally, but in batch-insert mode it cannot currently retrieve the data (except for the row first inserted through the view), resulting in enforcing the WITH CHECK OPTION constraints with the data passed from the core (except for the first-inserted row), which led to incorrect results when inserting into a view referencing a foreign table in which a remote BEFORE ROW INSERT trigger changes the rows inserted through the view so that they violate the view's WITH CHECK OPTION constraint. Also, the query inserting into the view caused an assertion failure in assert-enabled builds. Fix these by disabling batch insertion when inserting into such a view. Back-patch to v14 where batch insertion was added. Discussion: https://postgr.es/m/CAPmGK17LpbTZs4m4a_6THP54UBeK9fHvX8aVVA%2BC6yEZDZwQcg%40mail.gmail.com
statext_is_compatible_clause_internal() checked that the arguments of a ScalarArrayOpExpr are one Var and one Const, but it would allow cases where the Const was on the left. Subsequent uses of the clause are not expecting that and would suffer assertion failures or core dumps. mcv.c also had not bothered to cope with the case of a NULL array constant, which seems really unacceptably sloppy of somebody. (Although our tools failed us there too, since AFAIK neither Coverity nor any compiler warned of the obvious use-of-uninitialized-variable condition.) It seems best to handle that by having statext_is_compatible_clause_internal() reject it. Noted while fixing bug #17570. Back-patch to v13 where the extended stats code grew some awareness of ScalarArrayOpExpr.
Since v14, the extended stats machinery will try to estimate for otherwise-unsupported boolean expressions if they match an expression available from an extended stats object. mcv.c did not get the memo about this, and would spit up with "unknown clause type". Fortunately the case is easy to handle, since we can expect the expression yields boolean. While here, replace some not-terribly-on-point assertions with simpler runtime tests for lookup failure. That seems appropriate so that we get an elog not a crash if we somehow get to the new it-should-be-a-bool-expression code with a subexpression that doesn't match any stats column. Per report from Danny Shemesh. Thanks to Justin Pryzby for preliminary investigation. Discussion: https://postgr.es/m/CAFZC=QqD6=27wQPOW1pbRa98KPyuyn+7cL_Ay_Ck-roZV84vHg@mail.gmail.com
On closer inspection, mcv.c isn't as broken for ScalarArrayOpExpr as I thought. The Var-on-right issue is real enough, but actually it does cope fine with a NULL array constant --- I was misled by an XXX comment suggesting it didn't. Undo that part of the code change, and replace the XXX comment with something less misleading.
As usual, the release notes for older branches will be made by cutting these down, but put them up for community review first. Due to the out-of-cycle release of 14.4, there are a number of commits that appeared in 14.4 that are not yet shipped in the earlier branches. This draft repeats those release note entries for convenience in preparing the older-branch notes later. They'll be stripped out of the 14.5 section after that's done.
6 participants
same as #1567